CI for forward-porting GC3 patches to GC4 #147

ddeclerck · 2024-05-29T22:26:35Z

Follow-up of #146.

GitMensch · 2024-05-31T18:56:18Z

build_aux/bootstrap

@@ -96,12 +96,15 @@ autoreconf $AC_OPTS $MAINPATH > $msgs 2>&1; ret=$?
 # Filter aminclude_static as those are only used _within_ another
 #        check so reporting as portability problem is only noise.
 # This has the effect of redirecting some error messages to stdout. 
+# to be moved to the Makefile - currently only usable for bootstrap,
+# but should be done on autogen, too


A, that old TODO...

GitMensch

I suggest to wrap the commits again. From what I've inspected we need one refactor for integrating 4.x logic (you've spotted that well) nicely.

GitMensch · 2024-05-31T19:02:49Z

libcob/fileio.c

-	snprintf (file_open_env, (size_t)COB_FILE_MAX, "%s%s", "IO_", s);
-	if ((file_open_io_env = cob_get_env (file_open_env, NULL)) == NULL) {
-		snprintf (file_open_env, (size_t)COB_FILE_MAX, "%s%s", "io_", s);
+	if (f != NULL) {


When/why should f be null here?

Because some functions (open_cbl_file, cob_sys_delete_file, ...) were "improved" to perform file mapping in GC3 (rev 3944), by calling the cob_chk_file_mapping function, which does not take a cob_file argument in GC3 but does in GC4, and that function in turn calls cob_chk_file_env. Since these functions (open_cbl_file, cob_sys_delete_file, ...) do not use a cob_file object, I resorted to passing NULL and coping with that...

Is this okay for you @GitMensch ?

libcob/fileio.c

GitMensch · 2024-05-31T19:48:51Z

oops, hope I haven't broken the gitignore in f36dcda - if not then we likely should apply that to the gcos3x branch as well.

ddeclerck · 2024-05-31T21:21:13Z

I suggest to wrap the commits again. From what I've inspected we need one refactor for integrating 4.x logic (you've spotted that well) nicely.

Saw your message a bit late, added another commit in the meantime 😅

By wrapping up you mean, committing to SVN ? (after doing the requested modifications of course)

GitMensch · 2024-06-01T07:50:07Z

This approach is reasonable in general. ... So you did already check that this piece of code changed in a later commit? If you see a difference in the current code, then you can use "svn annotate" to check which commit did the change in this party of fileio.c to know what to merge before the refactoring. :-) Am 1. Juni 2024 00:07:13 MESZ schrieb OCP David Declerck ***@***.***>:

…

@ddeclerck commented on this pull request. > + /* apply COB_FILE_PATH if set (similar to ACUCOBOL's FILE-PREFIX) */ + if (file_paths) { + for(k=0; file_paths[k] != NULL; k++) { + snprintf (file_open_buff, (size_t)COB_FILE_MAX, "%s%c%s", + file_paths[k], SLASH_CHAR, file_open_name); + file_open_buff[COB_FILE_MAX] = 0; + if (access (file_open_buff, F_OK) == 0) { + break; + } +#if defined(WITH_CISAM) || defined(WITH_DISAM) || defined(WITH_VBISAM) || defined(WITH_VISAM) + /* ISAM may append '.dat' to file name */ + snprintf (file_open_buff, (size_t)COB_FILE_MAX, "%s%c%s.dat", + file_paths[k], SLASH_CHAR, file_open_name); + file_open_buff[COB_FILE_MAX] = 0; + if (access (file_open_buff, F_OK) == 0) { + snprintf (file_open_buff, (size_t)COB_FILE_MAX, "%s%c%s", + file_paths[k], SLASH_CHAR, file_open_name); + file_open_buff[COB_FILE_MAX] = 0; + break; + } +#endif + } + if (file_paths[k] == NULL) { + snprintf (file_open_buff, (size_t)COB_FILE_MAX, "%s%c%s", + file_paths[0], SLASH_CHAR, file_open_name); + file_open_buff[COB_FILE_MAX] = 0; + } + strncpy (file_open_name, file_open_buff, (size_t)COB_FILE_MAX); + } I remember why I haven't refactored that straight away : in case subsequent commits modify the same code, conflicts will be much easier to handle. I was thinking about keeping the refactoring for after all the patches are merged... -- Reply to this email directly or view it on GitHub: #147 (comment) You are receiving this because you commented. Message ID: ***@***.***>

ddeclerck · 2024-06-01T09:03:33Z

I tend to be overly "conservative". Indeed this piece of code is barely modified afterwards, so I'll do the refactoring.

ddeclerck · 2024-06-05T08:53:45Z

Is this okay to merge (@GitMensch) ?

GitMensch · 2024-06-05T14:37:29Z

I'll try to review this (late) evening.

GitMensch · 2024-06-06T05:38:21Z

looks_absolute should use "src", not file_open_name directly (merge issue?)

"apply_file_paths" should get that via argument as well and have a function comment that it writes to the global buffer. Then add a Changelog "extracted from xyz and also used in abc" to finish that last commit.

We either have to remember for later that we need to add a testcase for the new use or (potentially easier) also include it in the last commit as well.

ddeclerck · 2024-06-06T07:02:09Z

looks_absolute should use "src", not file_open_name directly (merge issue?)

This change is introduced in a later commit (3993).

"apply_file_paths" should get that via argument as well and have a function comment that it writes to the global buffer. Then add a Changelog "extracted from xyz and also used in abc" to finish that last commit.

Alright ; as for its output, should it write it through its argument or directly to file_open_name ?

We either have to remember for later that we need to add a testcase for the new use or (potentially easier) also include it in the last commit as well.

By "new use", de you mean the fact that we apply file paths to the complex case ?

GitMensch · 2024-06-06T12:48:20Z

By "new use", de you mean the fact that we apply file paths to the complex case ?

yes

This change is introduced in a later commit (3993).

good catch - then it is fine to leave as is; if you don't expect any big problem it would be nice to merge that in this bunch to commit that together, but a later bunch is fine as well

Alright ; as for its output, should it write it through its argument or directly to file_open_name ?

Depends on how other functions do it - it is best for now to mimic that (once the merge is completed we may revisit that part, but there are "some" commits left until we get there).

ddeclerck · 2024-06-08T10:01:57Z

I made the necessary changes.

This change is introduced in a later commit (3993).

good catch - then it is fine to leave as is; if you don't expect any big problem it would be nice to merge that in this bunch to commit that together, but a later bunch is fine as well

I find it more convenient to merge consecutive commits. If that's okay for you I could add to the current batch the next eligible commits until 3993 (that would be 6 commits: 3973, 3979, 3988, 3989, 3992 and 3993).

GitMensch · 2024-06-08T10:52:34Z

That batch is good to go :-)

ddeclerck · 2024-06-08T11:14:22Z

That batch is good to go :-)

Merged in SVN ;)

I see the next commits deal with translation files. Checking the history, it seems those files are usually just copied "as-is" from the GC3 branch to the trunk; is this correct ?

GitMensch · 2024-06-08T11:38:48Z

I see the next commits deal with translation files. Checking the history, it seems those files are usually just copied "as-is" from the GC3 branch to the trunk; is this correct ?

No, only new files are copied, the others left as-is; before a release I regenerate the files but the files are nearly completely maintained by the translation project.
So just record the merge, then copy over new files and svn add them, if there are any.

And of course

ddeclerck · 2024-06-08T13:54:13Z

Alright.

And of course

Hope this unfinished sentence did not have any vital info 😅

GitMensch · 2024-06-08T14:01:22Z

I can't remember any important info missing there.

codecov-commenter · 2024-06-08T14:17:10Z

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 74.11765% with 22 lines in your changes missing coverage. Please review.

Please upload report for BASE (gc4@5e53aa0). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
cobc/field.c	62.96%	7 Missing and 13 partials ⚠️
cobc/cobc.c	0.00%	0 Missing and 1 partial ⚠️
cobc/parser.y	95.00%	0 Missing and 1 partial ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@          Coverage Diff           @@
##             gc4     #147   +/-   ##
======================================
  Coverage       ?   65.61%           
======================================
  Files          ?       38           
  Lines          ?    67728           
  Branches       ?    18932           
======================================
  Hits           ?    44438           
  Misses         ?    16111           
  Partials       ?     7179

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ddeclerck · 2024-06-08T22:54:46Z

Quick question: I sometimes see alternative code for GC4 in #if 0 blocks; I guess I should implement those and drop the other branch, correct ?

I'm talking about those:

#if 0 /* TODO for 4.0: set the attributes from the field given outside on the stack */
		output ("cob_field *cob_fret, const int cob_pam");
#else
		output ("cob_field **cob_fret, const int cob_pam");
#endif

GitMensch · 2024-06-12T20:21:25Z

That's quite a bunch - any reason to not merge upstream?
Open issues you are aware of or special adjustments needed?

[we really need to get to commits that have someone else in the ChangeLogs...]

ddeclerck · 2024-06-12T21:35:57Z

That's quite a bunch - any reason to not merge upstream? Open issues you are aware of or special adjustments needed?

No good reason. It may be many commits, but the first batch had way more lines (this one is only +1,162 −904).
Anyways, I'll merge upstream if that's okay with you.

[we really need to get to commits that have someone else in the ChangeLogs...]

I'm looking forward to reaching commit 4614 - our first contribution to GC3 ;)

ddeclerck · 2025-01-15T17:13:14Z

@GitMensch This looks large enough already, I'll think I'll end this batch here.

GitMensch

The current bunch looks quite fine. Some issues are in the area of "oh yeah, we fixed that later" - but nothing that's important enough to be early checked in.

If you don't see anything that needs to be detail-checked (any issues during merge?) then that's good to go upstream as-is.

ddeclerck · 2025-01-18T04:43:53Z

The current bunch looks quite fine. Some issues are in the area of "oh yeah, we fixed that later" - but nothing that's important enough to be early checked in.

What would this issues be ?

If you don't see anything that needs to be detail-checked (any issues during merge?) then that's good to go upstream as-is.

Nothing that I can find of. Merging 5129 was just "slightly" more tricky due to some differences between 3.x and trunk (required more manual conflict resolution), but it didn't raise any issue, and the testsuite ran fine after this. There were some issues trying to merge 5131, but I stepped back and chosed to leave this one for the next batch - so this next batch might be a bit more tricky (and I'll tell you if I encounter any difficulty).

GitMensch · 2025-01-18T07:29:22Z

So up to svn now (issues would be mostly around TODO notes). Am 18. Januar 2025 05:44:14 MEZ schrieb OCP David Declerck ***@***.***>:

…

> The current bunch looks quite fine. Some issues are in the area of "oh yeah, we fixed that later" - but nothing that's important enough to be early checked in. What would this issues be ? > If you don't see anything that needs to be detail-checked (any issues during merge?) then that's good to go upstream as-is. Nothing that I can find of. Merging 5129 was just "slightly" more tricky due to some differences between 3.x and trunk (required more manual conflict resolution), but it didn't raise any issue, and the testsuite ran fine after this. There were some issues trying to merge 5131, but I stepped back and chosed to leave this one for the next batch - so this next batch might be a bit more tricky (and I'll tell you if I encounter any difficulty). -- Reply to this email directly or view it on GitHub: #147 (comment) You are receiving this because you were mentioned. Message ID: ***@***.***>

ddeclerck · 2025-01-19T22:54:08Z

I think this concludes this batch. Don't be impressed by the line count: actually a single commit added 2600 lines to a testsuite file ; other than that, there's only 600 new lines of code.

GitMensch · 2025-01-24T07:43:28Z

.github/workflows/ubuntu.yml

Why is that needed? We shouldn't have anything in here that requires updating the internal m4 files or libtool, do we?

Don't know why, but with the latest Ubuntu image, the CI stopped working - complaining about a libtool version mismatch. So I had to add the "install" parameter to the bootstrap script (see below) to regenerate the files, but then it complained it could not find autopoint - hence the extra package here.

See the current GC4 CI run.
Also not that the MacOS and MSYS2 CI do call bootstrap install.

GitMensch · 2025-01-24T07:45:12Z

.github/workflows/ubuntu.yml

+# Since the latest Ubuntu image, lcov fails complaining about negative branch counts,
+# and using fprofile-update=atomic as suggested does not help, so use the previous image
+#    runs-on: ubuntu-latest
+    runs-on: ubuntu-22.04


complain = error? I think this should be fixed on the gcos-gc3 branch - and not with a downgrade

Yes, blocking error. See Issue 212. I haven't been able to fix that, so I went with a downgrade so that I could continue focusing on the merge.
Note that this is only for the coverage part of the workflow.

GitMensch

apart from the workflow file (which cannot be part of this in any case), which we should fix in the gcos 3.x branch in any case, this bunch is good to go; nice to have the testcases in

note: I'm not sure when this all broke, but the increasing test failures of 4.x under Win32 are kind of concerning:

MSVC - delay load stuff for BDB does not work any more - I'm sure it did before
MSYS1 - a bunch of new failures
general (minor): Win32 adjustments for the testsuite / code to reduce false-positives seems to not be merged yet; maybe we can get those commits in early, maybe not

It could be good to take some time "soon" to work at least on some of the new failures (especially the delay-load stuff for ISAM, that's 4.x in any case).
In general it would be good to update the gcos4 branch to the newest CI definitions (they look different now) and adjust the expected test case failures (likely more for now).

ddeclerck · 2025-01-24T09:41:03Z

note: I'm not sure when this all broke, but the increasing test failures of 4.x under Win32 are kind of concerning:

I keep track of those numbers, they do not seem to have increased significantly (even reduced under MSVC).
For MSVC (release), there were 141 failures when I started the merge, 75 after merging 4689, 60 after 5050, and 59 now, all this while the number of tests increased by 161. In debug mode, the number of failed tests is higher (133) due to the runtime checker reporting possible data loss because of casts to smaller data types.
For MSYS1, there are indeed a few more failures (8 between commits 4999 and this last batch, for a total of 53), but the number of tests also increased by 112.

MSVC - delay load stuff for BDB does not work any more - I'm sure it did before

As far as I remember, it did not. When I started the merge, I wrote down some of the most recurring error messages in the testsuite logs, and in particular I had many occurrences of "error: I/O routine BDB cannot be loaded: module libcobdb-1.dll not found"

general (minor): Win32 adjustments for the testsuite / code to reduce false-positives seems to not be merged yet; maybe we can get those commits in early, maybe not

Those commits (5308, 5315-5318) were merged a while ago.
I could do a new pass on the MSVC Debug to silence the lossy casts - since those don't happen in 3.x I guess they occur in 4.x-specific code.

In general it would be good to update the gcos4 branch to the newest CI definitions (they look different now)

I think you made #190 for this ; is this okay to be merged (after a rebase) ?
EDIT: note that I'm not confident about touching the CI, each time I do that, it always takes me forever to get it to work correctly, and the current one seems sufficient for its purpose (i.e. checking for the merge)

ddeclerck added the DO NOT MERGE label May 29, 2024

ddeclerck changed the title ~~Dummy commit~~ CI for forward-porting GC3 patches to GC4 May 29, 2024

ddeclerck force-pushed the gc4 branch from dc9dda1 to b9778c3 Compare May 30, 2024 17:29

ddeclerck force-pushed the gc3_to_gc4 branch 3 times, most recently from 6900d8d to 1c95bd0 Compare May 30, 2024 21:21

GitMensch reviewed May 31, 2024

View reviewed changes

GitMensch requested changes May 31, 2024

View reviewed changes

ddeclerck force-pushed the gc3_to_gc4 branch from c2829b1 to 18626fb Compare May 31, 2024 20:45

ddeclerck force-pushed the gc3_to_gc4 branch from 05442df to 5174bb3 Compare June 8, 2024 09:39

ddeclerck force-pushed the gc3_to_gc4 branch from 5174bb3 to e31913b Compare June 8, 2024 14:14

ddeclerck force-pushed the gc3_to_gc4 branch from 8197ec8 to 369e5bd Compare January 15, 2025 16:33

ddeclerck requested a review from GitMensch January 16, 2025 23:19

GitMensch reviewed Jan 17, 2025

View reviewed changes

ddeclerck added 2 commits January 18, 2025 16:18

Merge SVN 5131

8c7f1cd

Merge SVN 5132

6507f30

ddeclerck force-pushed the gc3_to_gc4 branch from 2a0b91e to 4a04f1f Compare January 18, 2025 16:24

Merge SVN 5136

6efdaba

ddeclerck force-pushed the gc3_to_gc4 branch 5 times, most recently from d77633c to ae1f11f Compare January 18, 2025 20:05

Update Ubuntu Workflow

153cd88

ddeclerck force-pushed the gc3_to_gc4 branch from ae1f11f to 153cd88 Compare January 19, 2025 10:11

ddeclerck added 7 commits January 19, 2025 22:02

Merge SVN 5134

8eddd85

Merge SVN 5135

0adcfe1

Merge SVN 5141

55a884a

Merge SVN 5142

8fc9f2e

Merge SVN 5143

0c03235

Merge SVN 5144

c57e3a2

Merge SVN 5145

5e238ff

ddeclerck requested a review from GitMensch January 22, 2025 08:48

GitMensch reviewed Jan 24, 2025

View reviewed changes

CI for forward-porting GC3 patches to GC4 #147

Are you sure you want to change the base?

CI for forward-porting GC3 patches to GC4 #147

Conversation

ddeclerck commented May 29, 2024

GitMensch May 31, 2024

Choose a reason for hiding this comment

GitMensch left a comment

Choose a reason for hiding this comment

GitMensch May 31, 2024

Choose a reason for hiding this comment

ddeclerck May 31, 2024

Choose a reason for hiding this comment

ddeclerck Jun 3, 2024

Choose a reason for hiding this comment

GitMensch commented May 31, 2024

ddeclerck commented May 31, 2024 • edited Loading

GitMensch commented Jun 1, 2024 via email

ddeclerck commented Jun 1, 2024

ddeclerck commented Jun 5, 2024

GitMensch commented Jun 5, 2024

GitMensch commented Jun 6, 2024

ddeclerck commented Jun 6, 2024

GitMensch commented Jun 6, 2024

ddeclerck commented Jun 8, 2024

GitMensch commented Jun 8, 2024

ddeclerck commented Jun 8, 2024

GitMensch commented Jun 8, 2024

ddeclerck commented Jun 8, 2024 • edited Loading

GitMensch commented Jun 8, 2024

codecov-commenter commented Jun 8, 2024 • edited Loading

Codecov Report

ddeclerck commented Jun 8, 2024

GitMensch commented Jun 12, 2024

ddeclerck commented Jun 12, 2024 • edited by GitMensch Loading

ddeclerck commented Jan 15, 2025

GitMensch left a comment

Choose a reason for hiding this comment

ddeclerck commented Jan 18, 2025

GitMensch commented Jan 18, 2025 via email

ddeclerck commented Jan 19, 2025

GitMensch Jan 24, 2025

Choose a reason for hiding this comment

ddeclerck Jan 24, 2025

Choose a reason for hiding this comment

ddeclerck Jan 24, 2025 • edited Loading

Choose a reason for hiding this comment

GitMensch Jan 24, 2025

Choose a reason for hiding this comment

ddeclerck Jan 24, 2025 • edited Loading

Choose a reason for hiding this comment

GitMensch left a comment

Choose a reason for hiding this comment

ddeclerck commented Jan 24, 2025 • edited Loading

ddeclerck commented May 31, 2024 •

edited

Loading

ddeclerck commented Jun 8, 2024 •

edited

Loading

codecov-commenter commented Jun 8, 2024 •

edited

Loading

ddeclerck commented Jun 12, 2024 •

edited by GitMensch

Loading

ddeclerck Jan 24, 2025 •

edited

Loading

ddeclerck Jan 24, 2025 •

edited

Loading

ddeclerck commented Jan 24, 2025 •

edited

Loading